Fix More dropdown (html, aria, bootstrap classes) #1414

gabalafou · 2023-08-17T18:33:55Z

Note that this PR specifically does not use the combobox role or pattern because a dropdown in a nav bar is not, semantically speaking, a select-only combobox. Accessibility expert Adrian Roselli advises against using such patterns for navigation, and proposes a disclosure widget navigation pattern instead.

The ARIA APG Patterns site also has an example of the disclosure pattern in navigation, which this PR uses as a reference for the HTML markup. Essentially this meant replacing aria-haspopup with aria-controls.

drammock · 2023-08-17T19:00:59Z

hover styling is incorrect I think (see "API") in screenshot:

Also might be nice to unify the styling of this dropdown and the version switcher? Version switcher has borders between items and the hover background color change affects the whole area of the container. I think I'd opt for making this one look like the version switcher rather than vice-versa, but I don't have super strong feelings about it.

src/pydata_sphinx_theme/toctree.py

gabalafou · 2023-08-17T19:13:24Z

@drammock, you're too fast! :-D

You got to this PR before I finished self-reviewing it, haha.

Yeah, the dark theme styles will need to be fixed. I think it looks okay in light theme, though — minus one minor issue where the hover color clips the focus ring, which I was going to submit a separate PR for.

As for unifying the style of the nav links dropdown with the version switcher, I would only be a fan of doing that if it simplifies the code. Otherwise, I don't feel strongly that the styles for the two things need to be the same, since they are not semantically the same, as I mention in the PR description.

drammock · 2023-08-17T19:30:16Z

Yeah, the dark theme styles will need to be fixed. I think it looks okay in light theme, though — minus one minor issue where the hover color clips the focus ring, which I was going to submit a separate PR for.

ah, yeah, I see the focus ring issue now. FWIW that happens on the version switcher too, please do fix both (in a separate PR is OK with me):

As for unifying the style of the nav links dropdown with the version switcher, I would only be a fan of doing that if it simplifies the code. Otherwise, I don't feel strongly that the styles for the two things need to be the same, since they are not semantically the same, as I mention in the PR description.

IDK if it will simplify the code, but I think it makes the UX better for mouse users. The More Dropdown currently has gutters where there is no hover effect and no effect of clicking; IMO that's not a good thing to have in a dropdown menu. I also think that it's OK for two things that are technically semantically different to have similar appearance given that their interaction mechanisms are the same --- in both cases you click (or press enter when focused) to reveal some options, navigate those options with mouse or arrow keys or TAB, select an option with click or enter, and abort by clicking outside the menu or hitting escape.

gabalafou · 2023-08-17T19:50:19Z

ah, yeah, I see the focus ring issue now. FWIW that happens on the version switcher too, please do fix both

I created an issue (#1415) referencing your comment so we can keep track of the bug.

I also think that it's OK for two things that are technically semantically different to have similar appearance given that their interaction mechanisms are the same

I totally agree.

I wonder why that padding (gutters) you mention is there. At first, I thought it was just default Bootstrap styling, but then I saw from the Bootstrap dropdown docs page that that is not the case. It looks like the gutters were added in #754 but I'm not sure why.

drammock · 2023-08-17T20:01:15Z

It looks like the gutters were added in #754 but I'm not sure why.

I think it's safe to assume "no good reason" for adding the padding and you should feel free to remove it.

gabalafou · 2023-08-17T20:44:47Z

Okay, my last two commits should do the trick!

drammock

the background color change on hover is different from the version switcher menu. We have a specific rule for the version switcher (it gets --pst-color-surface) but for the More... links we're getting the color from Bootstrap's CSS. We should be consistent and IMO since @trallard put so much work into our palette we should use those colors instead of bootstrap's colors.

drammock · 2023-08-17T21:13:03Z

src/pydata_sphinx_theme/assets/styles/sections/_header.scss

@@ -107,10 +107,14 @@
        border: 1px solid var(--pst-color-border);
        box-shadow: 0 0 0.3rem 0.1rem var(--pst-color-shadow);
        background-color: var(--pst-color-on-background);
-        padding: 0.5rem 1rem;
+        padding: 0.5rem 0;


IMO we don't need the top/bottom padding either, but feel free to ignore this suggestion if you disagree

Suggested change

padding: 0.5rem 0;

padding: 0;

It's not that I agree or disagree. It's more that this PR doesn't exactly feel like the right place to change padding or override somebody else's prior aesthetic choice. The focus of this PR is about correcting (1) the AT (assistive tech, accessibility tree) semantics and (2) interaction mechanisms. Some of that necessarily entails changing some visual aspects of the UI, but padding is not one of those aspects.

But I will gladly open up a follow-up PR to put it on the table :)

src/pydata_sphinx_theme/toctree.py

gabalafou

Made some more changes, left some inline code comments.

I think the only visual differences now between this dropdown and the version switcher are the padding and the inbetween border lines.

gabalafou · 2023-08-18T13:40:22Z

src/pydata_sphinx_theme/assets/styles/sections/_header.scss

@@ -107,10 +107,14 @@
        border: 1px solid var(--pst-color-border);
        box-shadow: 0 0 0.3rem 0.1rem var(--pst-color-shadow);
        background-color: var(--pst-color-on-background);
-        padding: 0.5rem 1rem;
+        padding: 0.5rem 0;


It's not that I agree or disagree. It's more that this PR doesn't exactly feel like the right place to change padding or override somebody else's prior aesthetic choice. The focus of this PR is about correcting (1) the AT (assistive tech, accessibility tree) semantics and (2) interaction mechanisms. Some of that necessarily entails changing some visual aspects of the UI, but padding is not one of those aspects.

gabalafou · 2023-08-18T13:53:18Z

src/pydata_sphinx_theme/assets/styles/sections/_header.scss

+
+        // Override Bootstrap
+        .nav-link {
+          transition: none;


This one was making me go a little batty. Because at first I couldn't tell what was still different between this dropdown and the other, but then when I looked closer, I realized that the items in the version switcher dropdown do not have the nav-link class, which comes with a transition (on color, background-color, and border-color) from Bootstrap. But this meant that the {text-decoration: underline} of the links would occur ever so slightly faster than the color changes.

I think this was creating some weird effect, where it seemed like the text color change was lagging the roll-over/hover background color change or vice versa.

Turning off the transitions makes this dropdown match the other one. However, it means that the nav-link links in this dropdown do not transition their color like the ones before the dropdown in the nav bar.

In other words, with this change, if we have links A, B, C, with A and B in the nav bar outside the dropdown, and link C in the dropdown, then links A and B will have a subtle text color transition when you roll the mouse over them, whereas link C will not.

for consistency why not turn off the transition for all nav-links (not just the ones in the dropdown)?

tl;dr-- My opinion: we should either disable the transition, as you suggest @drammock, or find a way to get them consistent and in sync.

The reason I didn't set transition: none for all .nav-link elements is that I assume that most of the Bootstrap styles are generally desired, since Bootstrap is a mature library, and extensive thought has been put into the design details.

That said, your question made me realize something. We have added hover-underlining to the nav-link class. That's not something Bootstrap provides. In fact, we actually override Bootstrap's default behavior, which is to explicitly not underline nav links on hover.

hours later ...

😩

Turns out, getting the text underline to fade in and out, so that it looks in sync with the color transitions that Bootstrap puts on .nav-link elements, is really, really hard¹. But the reason it matters is that if the underline appears and disappears before the color transition finishes, even with only a 0.15s lag, I think it makes the overall effect just a tiny bit funky and off-putting, at least to my brain.

So... my recommendation: either we turn off the transition altogether, or we find a way to get the underline to animate (somehow) in sync with the 0.15s color transition.

cc @trallard because the git history shows that you have thought a lot about the link hover styles.

This is ultimately a microscopic design detail, but since it's come up in this PR, I feel like we might as well address it and come to a decision about what path we want to take. I created a Codepen to put together what I see as all the possible paths forward.

Footnotes

It's hard because text-decoration-style is a discrete animation property, meaning it goes on and off like a lightbulb; it's not natively transitionable. I tried using text-decoration-thickness, but it seems that you can't actually set a thickness of 0px? Or at least, when I did set thickness to 0, I got a really thin but not zero-width underline. I'm also not really sure if animating the line thickness—having the underline grow from thinner to thicker—is really a good way to make the color transition sync with the underline transition. (Also the transition just straight up doesn't seem to work in Chrome, even though it should? It does work in Firefox. 🤷‍♂️) I also tried using a :before pseudo-element, but that had all sorts of alignment issues that I wasn't willing to throw hours of time into trying to solve. I also tried animating a bottom border from transparent to a solid color, but that's also tricky to guarantee that the border only extends the length of the text (this is where text-decoration really outshines border-bottom). Maybe some CSS wizard knows the right incantation. I do not. ↩

Had a chat with @trallard and she's in favor of turning off all the .nav-link transitions for now.

That makes three of us, so I'm going to go ahead and push that change to this PR.

We can revisit this in the future once higher priority design and accessibility issues have been resolved.

gabalafou · 2023-08-18T13:56:37Z

src/pydata_sphinx_theme/assets/styles/sections/_header.scss

+          padding: 0.25rem 1.5rem;
+
+          // Override Bootstrap
+          &:focus:not(:hover):not(:active) {


Had to add the :not(:hover):not(:active), otherwise this focus rule overrides the Bootstrap-provided hover and active styles because the selector has higher specificity.

gabalafou · 2023-08-18T13:58:28Z

src/pydata_sphinx_theme/assets/styles/variables/_bootstrap.scss

+$dropdown-link-hover-bg: var(--pst-color-surface);
+// --pst-color-surface can also be assigned to the dark variant because it is
+// scoped to different values depending on light/dark theme
+$dropdown-dark-link-hover-bg: var(--pst-color-surface);
+$dropdown-link-active-bg: var(--pst-color-surface);
+$dropdown-dark-link-active-bg: var(--pst-color-surface);


Not sure if this is the right move, but it seemed that if we want to override the default dropdown styles in this dropdown, the we'd probably want to override them in any other dropdown that we might add in the future?

why do we need separate light/dark variables --- oh, is it because these are preexisting bootstrap variables that we're just overriding?

gabalafou · 2023-08-18T14:15:56Z

Thanks @drammock for bringing up the point about colors. I totally agree. I remapped the Bootstrap dropdown colors in my latest changes.

drammock

the linter CI is failing. Seems you don't have our pre-commit hooks set up?

https://pydata-sphinx-theme.readthedocs.io/en/latest/community/setup.html#setup-pre-commit

src/pydata_sphinx_theme/toctree.py

drammock · 2023-08-18T14:38:38Z

src/pydata_sphinx_theme/assets/styles/variables/_bootstrap.scss

+$dropdown-link-hover-bg: var(--pst-color-surface);
+// --pst-color-surface can also be assigned to the dark variant because it is
+// scoped to different values depending on light/dark theme
+$dropdown-dark-link-hover-bg: var(--pst-color-surface);
+$dropdown-link-active-bg: var(--pst-color-surface);
+$dropdown-dark-link-active-bg: var(--pst-color-surface);


why do we need separate light/dark variables --- oh, is it because these are preexisting bootstrap variables that we're just overriding?

drammock · 2023-08-18T14:43:11Z

src/pydata_sphinx_theme/assets/styles/sections/_header.scss

+
+        // Override Bootstrap
+        .nav-link {
+          transition: none;


for consistency why not turn off the transition for all nav-links (not just the ones in the dropdown)?

gabalafou · 2023-08-18T14:49:30Z

Seems you don't have our pre-commit hooks set up?

No, I do but it was the code comment I committed via GitHub suggestion that caused the CI lint job to fail. 🤦

drammock · 2023-08-22T13:53:22Z

at a quick glance I think the test failure relates to changing the dropdown from a div to a ul. @gabalafou can you fix? If you can't sort it LMK and I can try.

gabalafou · 2023-08-24T10:06:46Z

All checks pass now, I think this is ready to go

drammock

very clean fixes to the test, thanks!

Fix More dropdown (html, aria, bootstrap classes)

aedc6a2

gabalafou commented Aug 17, 2023

View reviewed changes

gabalafou mentioned this pull request Aug 17, 2023

Focus ring clipped by hover styles #1415

Closed

gabalafou added 2 commits August 17, 2023 23:19

update dropdowns when user changes light/dark theme

41f3b93

fix padding

1d28b04

drammock reviewed Aug 17, 2023

View reviewed changes

fix active, hover - colors, transition

a7f307c

trallard added the tag: accessibility Issues related to accessibility issues or efforts label Aug 18, 2023

gabalafou commented Aug 18, 2023

View reviewed changes

Update src/pydata_sphinx_theme/toctree.py

10dac40

drammock reviewed Aug 18, 2023

View reviewed changes

lint

eed8141

gabalafou mentioned this pull request Aug 18, 2023

Unprefixed IDs #1417

Open

trallard mentioned this pull request Aug 22, 2023

a11y weekly sync - 22/08/2023 Quansight-Labs/czi-scientific-python-mgmt#39

Closed

turn off all transitions on .nav-link

9648d66

gabalafou added 2 commits August 23, 2023 18:55

fix test

13761bb

make test more dry, readable

00e2fae

gabalafou mentioned this pull request Aug 24, 2023

Excess padding in nav bar dropdown #1421

Open

drammock approved these changes Aug 24, 2023

View reviewed changes

drammock merged commit f6e1943 into pydata:main Aug 24, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix More dropdown (html, aria, bootstrap classes) #1414

Fix More dropdown (html, aria, bootstrap classes) #1414

gabalafou commented Aug 17, 2023

drammock commented Aug 17, 2023

gabalafou commented Aug 17, 2023

drammock commented Aug 17, 2023

gabalafou commented Aug 17, 2023 •

edited

Loading

drammock commented Aug 17, 2023

gabalafou commented Aug 17, 2023

drammock left a comment

drammock Aug 17, 2023

gabalafou Aug 18, 2023

gabalafou Aug 18, 2023

gabalafou left a comment

gabalafou Aug 18, 2023

gabalafou Aug 18, 2023 •

edited

Loading

drammock Aug 18, 2023

gabalafou Aug 21, 2023

gabalafou Aug 22, 2023

gabalafou Aug 18, 2023

gabalafou Aug 18, 2023

drammock Aug 18, 2023

gabalafou Aug 18, 2023

gabalafou commented Aug 18, 2023

drammock left a comment

drammock Aug 18, 2023

drammock Aug 18, 2023

gabalafou commented Aug 18, 2023

drammock commented Aug 22, 2023

gabalafou commented Aug 24, 2023

drammock left a comment

Fix More dropdown (html, aria, bootstrap classes) #1414

Fix More dropdown (html, aria, bootstrap classes) #1414

Conversation

gabalafou commented Aug 17, 2023

drammock commented Aug 17, 2023

gabalafou commented Aug 17, 2023

drammock commented Aug 17, 2023

gabalafou commented Aug 17, 2023 • edited Loading

drammock commented Aug 17, 2023

gabalafou commented Aug 17, 2023

drammock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou commented Aug 18, 2023

drammock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou commented Aug 18, 2023

drammock commented Aug 22, 2023

gabalafou commented Aug 24, 2023

drammock left a comment

Choose a reason for hiding this comment

gabalafou commented Aug 17, 2023 •

edited

Loading

gabalafou Aug 18, 2023 •

edited

Loading